-
Notifications
You must be signed in to change notification settings - Fork 639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
iOS / macOS build fixes, iOS Simulator for x86_64 hosts, VS and Xcode build/run configs #1177
base: main
Are you sure you want to change the base?
Conversation
…ulkan_sample.h and hpp_hello_triangle
… builds for macOS
…VK vs. Vulkan loader
…imulator (arm64 & x86_64)
…, and default sample
…uild type is Release
Interesting, it seems the CI does not provide a Vulkan SDK for macOS Debug or Release builds, kind of unexpected. Also, it appears the CI does not specify a development team for iOS builds, again unexpected. Let me look deeper into this and I will come back with a proposal or solution. |
Can you elaborate? We do have CI with SDK for macOS iOS and both fetch the SDK. |
Maybe we could simplify things if you'd create an issue for changes like this first, so we can discuss these at our sample's team meeting. We talk a lot about build setups and also Apple platforms, but not always in public. Getting some notification upfront in the form of an issue would probably help us coordinate a bit more efficient ;) |
I'm not 100% convinced that this is something we want. What is the use-case here aside from running a self-built version of MoltenVK? I don't want the CMake setup become too complicated. It's already hard to maintain (and test) all of this, and I don't think we need to support all possible use-cases, esp. niche ones. |
I am not a member of your forum and can't participate in those kinds of planning discussions. And using github's issue tracker for more strategic stuff is not ideal. All I can really do is propose changes, and like most developers, we can see things better and respond if it is concrete. In the end it is up to you and your team whether to take such changes or reject them. I will not be offended if you choose not to proceed with certain ones. In the end I am just trying to help and provide a better experience for Apple developers using your project. I will fix the CI failure issues and then you can decide what parts if any you may take. I have been around the development world a long time and sometimes work has to be adapted or thrown away. No worries. |
@SaschaWillems is right in that we should optimize for reduced CMake complexity. Maybe to satisfy both needs, we could use options in CMake and set the default path to _HPP_VULKAN_LIBRARY as "vulkan.framework/vulkan" that way we'd be reducing the complexity at least in the dynamic loader. However, I ultimately think this change needs to happen in VULKAN_HPP project rather than us doing something specific per platform. |
Vulkan-Samples/.github/workflows/build.yml Line 144 in 537eb57
yep, looks like iOS does indeed use the wrong script. You should be able to fix it there. The MacOS one uses the same rules as the other Desktop OSes, it should be pulling down the VulkanSDK too. |
Agreed, it should be
I don't see VulkanSDK being pulled for any of the Desktop OSes. I guess the CI build is relying on Vulkan-Headers from the project. Not an emergency, and I can make small change that will avoid the CI issue for macOS builds. |
My proposed |
Okay. I think I have heard the following:
a) vulkan_sample.h already uses a hard-coded b) The support for MoltenVK dev builds was a freebie (cmake changes only) and a bit of a personal need. However, this may be a step too far for this project. I could remove that part from this PR, or at minimum remove it from the docs. I will start with the latter approach (i.e. remove from docs), and wait for further feedback.
I plan to push these changes today and hopefully the CI will pass. Following that, I await additional feedback on what to keep and what to toss out. |
…ect option from docs
75a72a1
to
2e0b325
Compare
I decided to proceed with the DynamicLoader However, clang-format is haunting me again. I run it before commiting, everything is clean, and yet it still complains during CI. I may have to move to Windows or Linux and try to force-push again. Not sure what is going on. clang-format UPDATE 1: I moved to Linux and ran clang-format with no issues. Then I went to Windows and undid the commit and the changes. Re-edited the changes inside Visual Studio. Downloaded LLVM and ran clang-format over there. Force-pushed from Windows and the same thing happened. Even the CI clang-format diffs message doesn't seem to make sense. I've spent more time trying to fix clang-format than making and testing the actual code change. I give up - perhaps someone who has more experience with clang-format could help or tell me what is wrong with commit e1ffff3. clang-format UPDATE 2: Well, I finally solved it by removing a clang-format change. I suspect there is a settings problem or defect in clang-format-diff.py when there is a spacing-only change on a line. I had to undo the spacing-only change made by clang-format in an earlier commit (417b4a5) and force push again. It's kind of frustrating when the tools don't work properly. |
e1ffff3
to
ec38a37
Compare
ec38a37
to
ddf412a
Compare
Description
This PR provides build fixes and some new build features as follows:
For iOS / macOS builds only:
_HPP_VULKAN_LIBRARY
define in vulkan_sample.h and hpp_hello_triangle.cpp. This provides the runtime infrastructure to support dynamic loading of a specified Vulkan library, e.g. a) the vulkan framework for iOS and b) SDK or development versions of MoltenVK. This follows the same approach as GLFW, with its optional define_GLFW_VULKAN_LIBRARY
.General improvements for Visual Studio Windows and Xcode macOS/iOS:
Forces VS, Xcode, Ninja Multi-Config to build for Release when the CMake build type is set to Release. This is accomplished by removing the Debug, and MinSizeRelease build types from the CMake config type variable when the build type is set to Release. The positive effect here is that when you open Visual Studio or Xcode, the correct build type will already be set along with the executable and default sample (from 1 above). All you need to do is build and run.This has been pretty thoroughly tested on Windows 10 (VS and command line builds), Manjaro Linux (command line builds), macOS (Xcode and command line builds), iOS (Xcode builds), and the iOS Simulator (Xcode builds). In addition, the Vulkan loader vs. MoltenVK optionality has also been tested for macOS, iOS, and iOS Simulator targets.
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including:
If this PR contains framework changes:
batch
command line argument to make sure all samples still work properlySample Checklist
If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist: